Claim/proof improvements #36
Conversation
bryanchriswhite
force-pushed
the
bryan/claim-proof-validation
branch
from
65e9b46 to
86193db
Compare
| @@ -12,6 +12,9 @@ message Session { | |||
| string session_id = 1; | |||
| uint64 session_number = 2; // The session number | |||
| uint64 session_block_start_height = 3; // The height at which the session started | |||
| // TODO_CONSIDERATION: do we really need this field? This should be a | |||
There was a problem hiding this comment.
True, we can probably remove it.
The ONLY thing I haven't thought through is how to handle changes in the number of blocks per session while a session is active.
There was a problem hiding this comment.
Had some thoughts about this. We could have a map of previous session numbers with their heights each time gov. changes this parameter (à la protocol upgrade).
Then if we want to check the height of a session we should calculate it using this map, eg.
[1, 10]: 20 // 2 blocks per session
[11, 15]: 40 // 11 to 15 had 4 blocks per session
If after session height 15, we have 5 blocks per session and we are at block 20 we do 40 + ((20 - 15) * 5) = 85 which is the block height for session 20. (don't mind off-by-one errors in the example if there are)
It also let us calculate the block height of any past session.
We may have a slightly more complex structure if we want different blocks_per_session per service.
There was a problem hiding this comment.
Let's leave this for a post-alpha discussion and just assume a constant (governance-driven) parameter for now. What you're describing will work but I want to see if we can make it simpler.
proto/poktroll/session/session.proto
Outdated
| @@ -12,6 +12,9 @@ message Session { | |||
| string session_id = 1; | |||
| uint64 session_number = 2; // The session number | |||
| uint64 session_block_start_height = 3; // The height at which the session started | |||
| // TODO_CONSIDERATION: do we really need this field? This should be a | |||
| // governance parameter & can't be trusted/used by the servicer | |||
| // msgServer/keeper. | |||
There was a problem hiding this comment.
| // msgServer/keeper. | |
| // msgServer/keeper. Need to think through what we want to do when | |
| // the number of blocks per session changes during an active session. |
proto/poktroll/servicer/tx.proto
Outdated
| @@ -33,8 +33,10 @@ message MsgClaim { | |||
| bytes smst_root_hash = 2; | |||
| // IMPROVE: move session_id into a new session_header field | |||
| string session_id = 3; | |||
| uint64 session_number = 4; | |||
| // TECHDEBT: expiration_height is not used right now and could be computed from on-chain data | |||
There was a problem hiding this comment.
| // TECHDEBT: expiration_height is not used right now and could be computed from on-chain data | |
| // TECHDEBT: invalidation_height is not used right now and could be computed from on-chain data |
| // TODO_THIS_COMMIT: factor all this out to a library pkg so that it can be | ||
| // reused in the client / relayer. | ||
| // TODO_CONSIDERATION: we can do this in terms of sessionId instead of | ||
| // lastEndedSessionStartHeight; however, it would require refactoring the |
There was a problem hiding this comment.
@bryanchriswhite I believe I see what you're getting at now:
- App { } # Independent
- Session { SessionHeader, Apps, Servicers } # Depenency on AppKeeper & ServicerKeeper
- Servicer { /* business logic using Session */ } # Dependency on SessionKeeperA session is mean't to be a library (e.g. match watchers with sessions in the future as well), and I can see it having a dependency on Portals in the future as well. Embedding it inside the Servicer feels incorrect.
What I think we should do is keep the Session separate for now (as it is right now) and keep seeing how many hacks/inconveniences we'll go through.
cc @red-0ne
There was a problem hiding this comment.
Will look into the different refactoring attempts then try to address this in a separate task
|
@red-0ne Please re-request a review on this PR when it's ready for a second look. |
|
Addressed the review comments. Will continue tightening the claim/proof flow making sure it works as intended. Will continue form this to add mint/burn logic |
Changes
poktrolld query servicer proofs [servicer-address]Claimprotobuf type, used for persistence between claim and proof message handlersTODO